Skip to content

Name anonymous export default functions and classes "default" in stack traces and inspect - #38517

Open
robobun wants to merge 3 commits into
mainfrom
farm/6c55974c/export-default-frame-name
Open

robobun wants to merge 3 commits into
mainfrom
farm/6c55974c/export-default-frame-name

Conversation

@robobun

@robobun robobun commented Aug 14, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • A stack frame inside an anonymous export default class { ... } renders as at new starDefault (p.mjs:1:55) until something reads the class's .name; after that the same frame renders at new default (...), which is also what node prints. D.name itself is always "default", so the frame name depends on evaluation order.
  • Same for export default class extends Base {} (synthesized constructor), export default () => ... and export default (function () { ... }), and on every surface that shows a frame name: error.stack, CallSite#getFunctionName() under Error.prepareStackTrace, and the frames Bun's own uncaught error / unhandled rejection printer writes to stderr.
  • Bun.inspect / console.log of the class or function prints [class starDefault], [class starDefault extends Base], [Function: starDefault], and a JSX element whose type is such a class prints <starDefault /> (node: [class default], [Function: default]). These stay wrong even after .name is read.
  • Cause: JSC binds an anonymous default export to the private *default* identifier (vm.propertyNames->starDefaultPrivateName, whose string is "starDefault") and only substitutes "default" when it reifies the name property (JSFunction::reifyName). Bun's name lookups use an already reified own name property when there is one and otherwise take the name from the executable, where it is still the raw identifier:
    • src/jsc/bindings/ErrorStackTrace.cpp: functionName(vm, codeBlock) and the FinalizerSafety::MustNotTriggerGC branch read ecmaName() directly; functionName(vm, globalObject, object) gets it through JSC::getCalculatedDisplayName, whose last fallback is ecmaName() too.
    • src/jsc/bindings/bindings.cpp: JSC__JSValue__getName (inspect of functions and classes, describe(Class) names) through JSC::getCalculatedDisplayName, and JSC__JSValue__getNameProperty (JSX tag names) through jsExecutable()->name(). That fallback is only reached when JSFunction::name() returned "", which it does precisely for *default*, so it could only ever produce "" or "starDefault".

Fix

  • Zig::functionNameForDisplay(vm, name) returns "default" when name is the *default* identifier's string and returns name unchanged otherwise. The five fallbacks above pass their result through it; nothing else about any lookup changes, so every other name comes out exactly as before (verified below for the builtin-module functions that the first revision of this PR got wrong).
  • The check is name.impl() == starDefaultPrivateName.impl(), the same comparison Identifier::operator== performs in JSFunction::reifyName. The private identifier's string is its own unique StringImpl, so a function that is really named starDefault is left alone.
  • Why this is correct: "default" is the name JSC itself reifies for these functions (JSFunction::reifyName and JSFunction::originalName apply the same substitution), so the lookups now return the same string before and after .name is materialized, and they match node for every shape above.
  • The fuller shape of this fix is in JSC: JSFunction::calculatedDisplayName / getCalculatedDisplayName (and the callers of ecmaName() in StackFrame, SamplingProfiler and the inspector) have the same missing substitution, which is also why console.log(new D()) still prints starDefault {} (JSObject::calculatedClassName, tracked in inspect: print instances of an anonymous export default class as "default" #38530) and why --cpu-prof and the debugger show the same name. Fixing it in the fork would cover all of those and reduce this PR to its tests; that needs a WebKit PR and a version bump, and the two direct ecmaName() reads in ErrorStackTrace.cpp are Bun's own code either way. This PR fixes the surfaces Bun formats itself now; when the fork applies the substitution, functionNameForDisplay becomes an identity function and can be deleted along with its five call sites.
  • Also fixed by the same change today, without a dedicated test here: the Received function starDefault suffix of native ERR_INVALID_ARG_TYPE messages, since determineSpecificType uses functionName(vm, globalObject, object); node errors: render a callable's name in determineSpecificType like node #38473 independently reworks that call site to read .name.
  • Verified:
    • test/js/bun/test/stack.test.ts ("anonymous export default is named 'default'"): error.stack for all four shapes plus a control function really named starDefault, CallSite#getFunctionName(), before and after .name is read, and the frames printed for an uncaught throw and an unhandled rejection (those go through the MustNotTriggerGC branch; confirmed by reverting only that line, which fails exactly those two cases). All three fail on the released bun with starDefault and pass with this change.
    • test/js/bun/util/inspect.test.js ("anonymous export default class and function are named 'default'"): [class default], [class default extends Base], [Function: default], <default />. Fails on the released bun, passes with this change.
    • test/js/bun/util/inspect.test.js ("functions from built-in modules do not inspect as their internal variable name"): Bun.inspect(zlib.gzip) stays [Function] and Bun.inspect(fs.promises.readdir) stays [AsyncFunction]. Passes on the released bun and with this change; fails on the first revision of this PR ([Function: fn], [AsyncFunction: wrapped]).
    • Bun.inspect of zlib.gzip, zlib.gunzipSync, fs.promises.readFile / writeFile, Readable.prototype.map, util.promisify, events.once, Bun.serve, Array.prototype.map, new Promise resolvers, functions with an empty or non-empty displayName, inferred names and bound functions is byte-identical between the released bun and this change.
    • test/js/node/v8/capture-stack-trace.test.js, test/js/bun/util/inspect-error.test.js, reportError.test.ts, error-name-preservation.test.ts, test/js/bun/test/describe.test.ts, test/js/node/util/{bun-inspect,custom-inspect}.test.*, the two prepare-stack-trace / bindings-stack-trace regression tests and the rest of inspect.test.js pass (the two "minified file" cases in inspect-error.test.js fail identically on main under debug builds, where BUN_DEBUG enables showPrivateScriptsInStackTraces). error-gc-test.test.js and inspect-error-leak.test.js, which stress these lookups under Bun.gc, pass under the debug ASAN build with their timeouts raised.

Background

  • *default*: the spec's name for the module-internal binding created by export default <anonymous declaration or expression>. JSC represents it with a private symbol whose description is starDefault; that symbol is also the StringImpl behind the identifier's string, which is what the identity comparison relies on.
  • ecmaName(): the name JSC's parser records on a function's executable for the purposes of the ECMAScript name property (for example f in const f = () => {}). JSFunction::name() returns the binding name instead, and returns "" for *default*.
  • Lazy name reification: JSC does not create a function's own name property until it is first looked up. Bun's lookups read that property when it exists (so Object.defineProperty(fn, "name", ...) wins) and otherwise fall back to the executable, which is why the output used to depend on whether .name had been read.
  • FinalizerSafety::MustNotTriggerGC: Bun formats some stack traces while JSC is finalizing an error, or when printing an error that still owns its raw frames, where running getters or allocating on the JS heap is not allowed. That branch reads structure slots directly; this PR only wraps its existing ecmaName() return, which allocates nothing (defaultKeyword.string() is a VM-lifetime string).
First revision of this PR (superseded)

The first revision replaced JSC::getCalculatedDisplayName in both ErrorStackTrace.cpp and JSC__JSValue__getName with a Bun copy of the displayName / name / ecmaName chain that used the stack-trace rule for builtin functions (isHostFunction() instead of JSC's isHostOrBuiltinFunction()). Functions defined in Bun's built-in modules are JSC builtins, so Bun.inspect started printing their inferred internal names: zlib.gzip became [Function: fn] and the fs.promises wrappers became [AsyncFunction: wrapped]. Review caught it; the current revision leaves every lookup as it was and only post-filters the result, and the new builtin-module test pins that output.

…k traces and inspect

JSC binds an anonymous `export default` to the private `*default*`
identifier, whose string form is "starDefault". JSFunction::reifyName maps
it to "default" when the `name` property is materialized, but the
fallbacks Bun uses when that has not happened yet (ecmaName() in
ErrorStackTrace.cpp, JSC::getCalculatedDisplayName, and the executable
name in JSC__JSValue__getNameProperty) returned the raw identifier, so a
frame rendered as `at new starDefault (...)` until something read `.name`,
and Bun.inspect printed `[class starDefault]`.

Route every one of those fallbacks through one helper that renders the
`*default*` identifier as "default", and share the displayName/name chain
between the regular and the finalizer-safe frame name lookups instead of
keeping two copies of it.
@coderabbitai

coderabbitai Bot commented Aug 14, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 27 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: da22cfa4-558c-49ab-9e78-7aaecaf2c40e

📥 Commits

Reviewing files that changed from the base of the PR and between 7cf6296 and cfd9586.

📒 Files selected for processing (5)
  • src/jsc/bindings/ErrorStackTrace.cpp
  • src/jsc/bindings/ErrorStackTrace.h
  • src/jsc/bindings/bindings.cpp
  • test/js/bun/test/stack.test.ts
  • test/js/bun/util/inspect.test.js

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Aug 14, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status

Reproduced on bun 1.4.0 (linux x64) with a two-file ESM fixture: export default class { constructor() { this.err = new Error() } } imported from another module prints at new starDefault (...) for the constructor frame until D.name is read, then at new default (...); Bun.inspect(D) prints [class starDefault] regardless. Node prints default in both places.

Fix is in this PR (#38517): the five places that take a function's name from its executable (ErrorStackTrace.cpp, JSC__JSValue__getName, JSC__JSValue__getNameProperty) pass it through one helper that renders JSC's internal *default* binding as default; nothing else about the lookups changes. Tests: test/js/bun/test/stack.test.ts and test/js/bun/util/inspect.test.js; the new cases fail on the released bun and pass with this change, plus a test pinning the inspect output of builtin-module functions, which the first revision of this PR had changed.

Not covered here: console.log(new D()) still prints starDefault {}; that name is computed inside WebKit (JSObject::calculatedClassName) and is handled in #38530. The fuller fix for the whole family is in the WebKit fork (JSFunction::calculatedDisplayName / getCalculatedDisplayName); once that lands, the helper added here becomes an identity function and can be deleted.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this PR and didn't find any bugs. Because it refactors the C++ JSC name-lookup paths — including the FinalizerSafety::MustNotTriggerGC branch — and intentionally shifts Bun.inspect output for the empty-displayName and empty-name builtin cases, a human look at the GC-safety and behavioral-unification claims would still be worthwhile.

What was reviewed:

  • calculatedDisplayName uses only getConcurrently/getDirect/tryGetValueWithoutGC/nameWithoutGC and identifier reads — no JS-heap allocation or getter execution, matching the finalizer-safe contract of the inline code it replaces.
  • The hoisted FunctionNameFlags::Function write in the MustNotTriggerGC branch fires on the same set of callee types as the old per-return lambda.
  • functionNameForDisplay compares against starDefaultPrivateName (the private identifier), so a user function literally named starDefault is unaffected — covered by the named-star-default.mjs control case.
  • Tests follow harness conventions (tempDir, bunEnv, concurrent pipe drain, exact-value assertions on structured output).
Extended reasoning...

Overview

The PR fixes anonymous export default functions and classes rendering as starDefault in stack frames and Bun.inspect before their .name property is reified. It adds two helpers in ErrorStackTrace.cpp — functionNameForDisplay(vm, identifier) (maps the private *default* identifier to "default") and calculatedDisplayName(vm, object) (unified own-displayName → nameWithoutGC → mapped ecmaName chain) — and routes four previously-separate lookups through them: functionName(vm, codeBlock), functionName(vm, globalObject, object), the MustNotTriggerGC branch of the frame-based functionName, plus JSC__JSValue__getName and the jsExecutable()->name() fallback in JSC__JSValue__getNameProperty in bindings.cpp. Tests cover error.stack, CallSite#getFunctionName, uncaught/unhandled-rejection frame printing, and Bun.inspect for class/subclass/arrow/JSX shapes.

Security risks

None identified. This is display-name computation for diagnostics; no untrusted input parsing, auth, or network paths are touched.

Level of scrutiny

High. Per REVIEW.md, native JSC bindings are the most-blocked category, and this change specifically rewrites the FinalizerSafety::MustNotTriggerGC path — code that runs during error finalization where running getters or allocating on the JS heap is UB. The new calculatedDisplayName is asserted to preserve those guarantees (structure-slot reads only via getConcurrently + getDirect + tryGetValueWithoutGC, nameWithoutGC, and Identifier::string()), and traces cleanly to me, but a maintainer with JSC internals context should confirm. The change also replaces JSC::getCalculatedDisplayName (which had a TOP_EXCEPTION_SCOPE wrapper because it could enter JS) with the GC-free helper in the non-finalizer functionName path and in JSC__JSValue__getName — a semantic shift the PR description acknowledges (empty-string displayName now falls through; empty-name builtins fall through to inferred name), described as moving toward node's output.

Other factors

The PR description is unusually thorough: it names the mechanism (JSFunction::reifyName), enumerates every affected call site, documents what is intentionally not fixed (JSObject::calculatedClassName inside WebKit for new D()), and lists both new tests and the existing suites re-run under debug ASAN. The tests include a control case for a real function named starDefault, before/after .name reification, and the finalizer-safe path via uncaught-throw / unhandled-rejection subprocesses. The refactor deduplicates three hand-written copies of the name-lookup chain, which is exactly what REVIEW.md's "fix the whole class" guidance asks for — but that same unification is why this exceeds the "simple, mechanical, or obvious" bar for auto-approval.

@robobun

robobun commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator Author

Two notes for whoever picks up the points the review above asks a human to look at:

  • GC safety: the non-finalizer path did not lose anything by dropping the TOP_EXCEPTION_SCOPE. JSC::getCalculatedDisplayName is itself written for the "mutator isn't running" case (structure getConcurrently, getDirect, tryGetValueWithoutGC, nameWithoutGC, InternalFunction::name() on a non-rope string); it cannot throw, so the scope around it was vestigial. Zig::calculatedDisplayName makes the same calls, and every one of them was already made by the inline MustNotTriggerGC code this PR replaces, so the finalizer path runs the same set of operations as before, just through one function.
  • The two Bun.inspect corners, checked against the release build and this branch: f.displayName = "" goes from [Function] to [Function: f] (node: [Function: f]); the anonymous builtins a user can get hold of (new Promise(executor) resolve/reject, Promise.withResolvers()) still print [Function], since JSC's builtins give them no inferred name either. Named builtins (Array.prototype.map, Promise.prototype.then), non-empty displayName, inferred names, and bound functions are unchanged.

…reimplementing getCalculatedDisplayName

The previous revision replaced JSC::getCalculatedDisplayName with a local
copy of the displayName/name chain that used Bun's stack-trace rule for
builtin functions. JSC__JSValue__getName therefore started printing the
inferred ecmaName of functions defined in Bun's built-in modules, which are
JSC builtins: Bun.inspect(zlib.gzip) became "[Function: fn]" and
fs.promises wrappers became "[AsyncFunction: wrapped]".

Keep every existing lookup as it was and pass its result through
functionNameForDisplay(), which swaps the private *default* identifier's
string (compared by StringImpl identity) for "default". Add a test pinning
the inspect output of builtin-module functions.
Comment thread src/jsc/bindings/ErrorStackTrace.cpp Outdated
Comment thread src/jsc/bindings/ErrorStackTrace.h Outdated
@robobun

robobun commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator Author

Pushed a reduced revision (1dda628, cfd9586), description updated to match.

Self-review of the first revision found that replacing JSC::getCalculatedDisplayName with a local copy of the name chain changed Bun.inspect for functions defined in Bun's built-in modules (they are JSC builtins, so zlib.gzip became [Function: fn] and the fs.promises wrappers [AsyncFunction: wrapped]). The current revision keeps every lookup as it was and only passes the five executable-derived results through functionNameForDisplay, which swaps the *default* identifier's string for default. The src diff is now one helper plus five wrapped returns; inspect.test.js gained a case pinning the builtin-module output, which fails on the first revision and passes on the released bun and on this one.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this PR and didn't find any bugs. The native change is minimal — one identity-compare post-filter applied at five existing return sites — but it sits in the JSC bindings for error stack formatting and inspect, including the MustNotTriggerGC path, so a human look would still be worthwhile.

What was reviewed:

  • functionNameForDisplay in the MustNotTriggerGC branch: defaultKeyword.string() is a VM-lifetime CommonIdentifiers string and the check is a pointer compare, so no JS-heap allocation is introduced there.
  • Identity vs equality: starDefaultPrivateName is a unique SymbolImpl, so a user function literally named starDefault has a different impl() and is left alone (covered by the named-star-default.mjs control).
  • The JSC__JSValue__getNameProperty fallback wrap only fires after function->name(vm) returned empty for a non-host/non-builtin function, so builtin-module functions are unaffected (pinned by the new gzip/readdir test).
Extended reasoning...

Overview

The PR fixes anonymous export default functions/classes rendering as starDefault in stack traces, CallSite#getFunctionName(), Bun.inspect, and JSX element names. It adds Zig::functionNameForDisplay(vm, name) in ErrorStackTrace.cpp/.h — an 8-line helper that returns "default" when name.impl() is pointer-identical to vm.propertyNames->starDefaultPrivateName.impl(), otherwise returns the input unchanged — and wraps five existing name-lookup return values through it (three in ErrorStackTrace.cpp, two in bindings.cpp). ~160 lines of tests are added to stack.test.ts and inspect.test.js.

Security risks

None. This is display-name formatting only; no parsing of untrusted input, no allocation-size arithmetic, no auth/crypto/permissions.

Level of scrutiny

Medium-high. Although the diff is small and the helper is a pure post-filter, it touches core JSC binding code in ErrorStackTrace.cpp (used by every error stack, including the finalizer-safe path where GC must not be triggered) and bindings.cpp (JSC__JSValue__getName/getNameProperty, used by Bun.inspect). The MustNotTriggerGC invariant is exactly the kind of thing a maintainer should confirm; the analysis holds (both propertyNames reads are pointer/refcount operations on VM-lifetime atomized strings), but I'd rather a human sign off on it than approve JSC-bindings changes autonomously.

Other factors

  • The PR already went through one self-correction: the first revision reimplemented getCalculatedDisplayName and regressed builtin-module inspect output; the current revision reduced to a post-filter and added a test pinning Bun.inspect(zlib.gzip) / Bun.inspect(fs.promises.readdir).
  • Test coverage is thorough: all four export-default shapes, before/after .name reification, a starDefault-named control for the identity check, both the error.stack and the uncaught-error-printer code paths, and the JSX tag-name path. Tests follow harness conventions (tempDir, concurrent pipe drains, combined-object assertions).
  • The comment-cop bot's two "paragraph-long comment" flags were addressed in cfd9586 (both threads resolved).
  • The PR description notes a fuller fix belongs in the WebKit fork (getCalculatedDisplayName / calculatedClassName, tracked in #38530); that layering call is worth a maintainer's opinion.

@robobun

robobun commented Oct 8, 2026

Copy link
Copy Markdown
Collaborator Author

This PR conflicts with main now. ErrorStackTrace.cpp changed under it (ecmaNameWithoutGC, no lexicalGlobalObject parameter). Commit 4e1498f on branch robobun/da68edbd/anonymous-default-export-name has the same mapping on current main. It also has the JSC__JSValue__getClassName line of #38530, so an instance prints default {}.

Two groups of readers still show the private name after the mapping. I checked them on a release build of main bd599f5 with the mapping applied:

  • Readers of JSFunction::name, which is "" for *default*. mock(fn).name and getMockName() give "" for an anonymous default arrow (JSMockFunction.cpp:296). v8::Function::GetName() reads the same call (V8Function.cpp:123, read, not run).
  • Names that JavaScriptCore composes itself. Heap snapshot class names give starDefault for an instance of an anonymous default class. --cpu-prof gives starDefault for a frame that is inlined into its caller.

So the rule belongs one layer down, in the display name fall-throughs of the WebKit fork (getCalculatedDisplayName and JSFunction::calculatedDisplayName in JSFunction.cpp, and the raw ecmaName reads in SamplingProfiler.cpp and StackFrame.cpp). With the rule there, Bun needs the mapping only where it reads the name of the executable itself.

#34932 depends on this. It moves every anonymous default function from <file>_default onto *default*, so each reader above changes for functions too.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant